Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Handle EmptyRelation during SQL unparsing #10803

Merged
merged 5 commits into from
Jun 5, 2024
Merged

Handle EmptyRelation during SQL unparsing #10803

merged 5 commits into from
Jun 5, 2024

Conversation

goldmedal
Copy link
Contributor

Which issue does this PR close?

Closes #10799 .

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Yes

Are there any user-facing changes?

No

@github-actions github-actions bot added the sql SQL Planner label Jun 5, 2024
devinjdangelo
devinjdangelo approved these changes Jun 5, 2024
View reviewed changes
Copy link
Contributor

@devinjdangelo devinjdangelo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @goldmedal this looks good to me 🚀. I have one minor suggestion to avoid a panic.

datafusion/sql/src/unparser/ast.rs Outdated
@@ -242,8 +242,8 @@ impl SelectBuilder {
from: self
.from
.iter()
.map(|b| b.build())
.collect::<Result<Vec<_>, BuilderError>>()?,
.filter_map(|b| b.build().ok().unwrap())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this change will result in a panic on a UninitializedFieldError due to the unwrap. E.g. if self.relation is None rather than Some(None). I think we could avoid this by using Result::transpose rather than .ok().unwrap() here and then continuing to collect::<Result<T>, _>.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @devinjdangelo. Nice idea. I've learned more about Rust.

alamb
alamb approved these changes Jun 5, 2024
View reviewed changes
Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great to me -- thank you @goldmedal and @devinjdangelo

datafusion/sql/tests/cases/plan_to_sql.rs
@@ -78,6 +78,11 @@ fn roundtrip_expr() {
#[test]
fn roundtrip_statement() -> Result<()> {
let tests: Vec<&str> = vec![
"select 1;",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❤️

datafusion/sql/src/unparser/ast.rs
Comment on lines +371 to +373
let new = self;
new.relation = Some(TableFactorBuilder::Empty);
new
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you could also do this (which is more concise)

Suggested change
let new = self;
new.relation = Some(TableFactorBuilder::Empty);
new
self.relation = Some(TableFactorBuilder::Empty);
new

Though I see it follows the same pattern as the code above so if we are going to change any of the we should change them all

datafusion/sql/src/unparser/plan.rs
@@ -389,7 +389,10 @@ impl Unparser<'_> {
)?;

let ast_join = ast::Join {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is totally fine, but I figured I would point out another way to express this logic that you might prefer in future PRs

Instead of

   let ast_join = ast::Join {
                    relation: match right_relation.build()? {
                        Some(relation) => relation,
                        None => return internal_err!("Failed to build right relation"),
                    },
                    join_operator: self
                        .join_operator_to_sql(join.join_type, join_constraint),
                };

You can do something like this with a let .. else to match or return

   let Some(relation) = match right_relation.build()? else {
    return internal_err!("Failed to build right relation"),
   };

   let ast_join = ast::Join {
                    relation,
                    join_operator: self
                        .join_operator_to_sql(join.join_type, join_constraint),
                };

@alamb alamb merged commit c5cefa8 into apache:main Jun 5, 2024
23 checks passed
@goldmedal goldmedal deleted the feature/unparse-empty-relation branch June 5, 2024 17:23
@goldmedal
Copy link
Contributor Author

Thanks @alamb @devinjdangelo
I think I will address @alamb's comments in the follow-up PR.

@goldmedal goldmedal mentioned this pull request Jun 6, 2024
Merged
findepi pushed a commit to findepi/datafusion that referenced this pull request Jul 16, 2024
@goldmedal @findepi
Handle EmptyRelation during SQL unparsing (apache#10803)
9ed7753 
* handle empty relation for sql unparsing

* revert the doc test change

* remove dbg macro

* cargo fmt

* improve the error handling
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@devinjdangelo devinjdangelo devinjdangelo approved these changes

@alamb alamb alamb approved these changes

Assignees
No one assigned
Labels
sql SQL Planner
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support convert LogicalPlan::EmptyRelation to SQL String
3 participants
@goldmedal @alamb @devinjdangelo